PoC Bucketed rate limit #168

noblepaul · 2023-11-20T19:04:50Z

A query needs to be passed with the following header:
Solr-Request-Type: QUERY
As per below configuration, a query should be passed with a query parameter ?q-bucket=test-bucket
Example clusterprops.json:

    {
      "rate-limiters": {
        "readBuckets": [
        {
        "name": "test-bucket",
        "conditions": [{
          "queryParamPattern": {
            "q-bucket": "test-bucket"
          }
        }],
        "allowedRequests": 2,
        "slotAcquisitionTimeoutInMS": 1000
      }

        ]
      }
    }

chatman · 2023-12-04T19:45:30Z

@noblepaul The tryWaitAverage is always 0, there's a possible bug.

hiteshk25 · 2023-12-08T00:14:10Z

Couple of questions

is ?q-bucket=test-bucket is enough. it's not clear to me the need of request header
probably we can define two buckets foreground and background ?
when we can have multiple conditions ?
also "slotBorrowingEnabled":true feature is there? this seems interesting (https://solr.apache.org/guide/solr/latest/deployment-guide/rate-limiters.html)

noblepaul · 2024-01-16T20:20:14Z

is ?q-bucket=test-bucket is enough. it's not clear to me the need of request header
actually both are required.

The request header requirement is something we may be able to get rid of. the only reason it is there is because the current design requires that

probably we can define two buckets foreground and background ?

can you please elaborate?

when we can have multiple conditions ?
you could add multiple conditions . it's not very useful but , it's just there for completeness

also "slotBorrowingEnabled":true feature is there? this seems interesting

this is not done. let me explore that

hiteshk25 · 2024-01-16T21:40:16Z

The request header requirement is something we may be able to get rid of. the only reason it is there is because the current design requires that

if it not require then just remove it.

probably we can define two buckets foreground and background ?
can you please elaborate?

We have two types of requests, one from customers and other is background job. Thus wondering we can create two buckets for this, where customer request will get more preference but background job should not execute query requests more than n.

when we can have multiple conditions ?
you could add multiple conditions . it's not very useful but , it's just there for completeness

Same as above if not require just remove it. I think one param is more than enough?

also "slotBorrowingEnabled":true feature is there? this seems interesting
this is not done. let me explore that

that maybe helpful in two bucket scenario

patsonluk · 2024-01-18T03:21:48Z

solr/core/src/java/org/apache/solr/core/RateLimiterConfig.java

@@ -31,6 +33,7 @@ public class RateLimiterConfig {
  public int allowedRequests;
  public boolean isSlotBorrowingEnabled;
  public int guaranteedSlotsThreshold;
+  public List<RateLimiterPayload.ReadBucketConfig> readBuckets;


My guess of readBuckets nested inside RateLimiterConfig is to allow the json reader to handle both cases:

no bucket, just configuration on the existing non bucket limiter:

"rate-limiters": { "allowedRequests": 2, "slotAcquisitionTimeoutInMS": 1000 }

use bucket:

"rate-limiters": { "readBuckets": [ { "name": "test-bucket", "conditions": [{ "queryParamPattern": { "q-bucket": "test-bucket" } }], "allowedRequests": 2, "slotAcquisitionTimeoutInMS": 1000 }] }

However, this could introduce some confusion as properties such as allowedRequests could be defined either under each "readBuckets" and/or "rate-limiters".

The allowedRequests defined under "rate-limiters" will be ignored if there are buckets. It seems all fields in RateLimiterConfig will be ignored except readBuckets, which lead me to think whether we should just use 2 separate class/config to avoid confusion and unused fields.

Could we just use something like check existence of rate-limiters and rate-limiters-buckets key to identify whether it's bucket and non-bucket use case (I think they are mutually exclusive) For example:

No bucket usage

"rate-limiters": { "allowedRequests": 2, "slotAcquisitionTimeoutInMS": 1000 }

or
2. Bucket usage

"rate-limiters-buckets" : [ { "name": "test-bucket", "conditions": [{ "queryParamPattern": { "q-bucket": "test-bucket" } }], "allowedRequests": 2, "slotAcquisitionTimeoutInMS": 1000 } ]

With this proposal the first case will be deserialized as RateLimiterConfig, and the 2nd case will just be parsed as an array of ReadBucketConfig, which might affect the ctor signature of RequestRateLimiter (and child classes), more on this later.

I thought about it. This was done to simplify the code. Isn't it better to just warn the user if other properties are used?

patsonluk · 2024-01-18T03:48:51Z

solr/core/src/java/org/apache/solr/servlet/BucketedQueryRateLimiter.java

+
+  @SuppressWarnings("unchecked")
+  public BucketedQueryRateLimiter(RateLimiterConfig rateLimiterConfig) {
+    super(rateLimiterConfig);


Perhaps we can consider enhancing the RequestRateLimiter logic in the manager at here, instead of introducing BucketQueryRateLimiter? It could avoid some code duplication, and unused Semaphore in parent class QueryRateLimiter.

It appears that we want the same ability as the existing QueryRateLimiter in terms of throttling (or even borrowing as @hiteshk25 mentioned in another comment), so it might be handy to just re-use QueryRateLimiter/RequestRateLimiter to handle bucketed limiter as well?

The idea is during the lookup, we could use a Matcher to return the correct Rate limiter. For example for non bucket configuration, it will just be the existing code (using RequestType for simple lookup from the map), but for bucket configuration, it will use iterate and check for match similar to here? :)

And I ❤️ the new metrics added, perhaps we can add that to QueryRateLimiter instead of just this class?

solr/core/src/java/org/apache/solr/servlet/BucketedQueryRateLimiter.java

noblepaul · 2024-01-18T04:00:37Z

@hiteshk25

The request header requirement is something we may be able to get rid of. the only reason it is there is because the current design requires that
if it not require then just remove it.

According to the current design, the feature kicks in only if the header is present. if we remove it, it may not be backward compatible

We have two types of requests, one from customers and other is background job. Thus wondering we can create two buckets for this, where customer request will get more preference but background job should not execute query requests more than n.

There is already an implicit bucket which has an unlimited no:of threads. Only explicit bucket is just throttled. So, we should create a just a bucket for background tasks and limit the no:of operations and leave the rest as is

hiteshk25 · 2024-01-18T20:17:32Z

@noblepaul

According to the current design, the feature kicks in only if the header is present. if we remove it, it may not be backward compatible

Nice, then can we just accommodate header only?

hiteshk25 · 2024-01-22T18:30:56Z

According to the current design, the feature kicks in only if the header is present. if we remove it, it may not be backward compatible
looks like it not documented if the header is present? Or I can't find it

noblepaul · 2024-01-22T19:46:25Z

Nice, then can we just accommodate header only?

You mean, do not support parameters ?

hiteshk25 · 2024-01-22T19:56:34Z

Nice, then can we just accommodate header only?

You mean, do not support parameters ?

I want to understand current behavior(limitation, etc)- how this get triggered(http header?)
based on above http header will work to point the new bucket?

bucketed rate limiter implemented

cb38f31

noblepaul marked this pull request as draft November 20, 2023 19:05

noblepaul added 6 commits November 21, 2023 06:22

bucketed rate limiter implemented

82374d2

test changes

89ebb08

tidy

0202d69

added metrics

8a07949

fixed metrics

ba7c207

added test

1b796d7

chatman marked this pull request as ready for review December 4, 2023 19:38

chatman requested review from patsonluk, magibney, hiteshk25, justinrsweeney and nginthfs December 4, 2023 19:46

hiteshk25 self-assigned this Dec 8, 2023

patsonluk reviewed Jan 18, 2024

View reviewed changes

solr/core/src/java/org/apache/solr/servlet/BucketedQueryRateLimiter.java Outdated Show resolved Hide resolved

if even a single condition fails return false

3e31f1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC Bucketed rate limit #168

PoC Bucketed rate limit #168

noblepaul commented Nov 20, 2023 •

edited by chatman

Loading

chatman commented Dec 4, 2023

hiteshk25 commented Dec 8, 2023 •

edited

Loading

noblepaul commented Jan 16, 2024

hiteshk25 commented Jan 16, 2024

patsonluk Jan 18, 2024 •

edited

Loading

noblepaul Jan 22, 2024 •

edited

Loading

patsonluk Jan 18, 2024 •

edited

Loading

noblepaul commented Jan 18, 2024

hiteshk25 commented Jan 18, 2024

hiteshk25 commented Jan 22, 2024

noblepaul commented Jan 22, 2024

hiteshk25 commented Jan 22, 2024

PoC Bucketed rate limit #168

Are you sure you want to change the base?

PoC Bucketed rate limit #168

Conversation

noblepaul commented Nov 20, 2023 • edited by chatman Loading

chatman commented Dec 4, 2023

hiteshk25 commented Dec 8, 2023 • edited Loading

noblepaul commented Jan 16, 2024

hiteshk25 commented Jan 16, 2024

patsonluk Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

noblepaul Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

patsonluk Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

noblepaul commented Jan 18, 2024

hiteshk25 commented Jan 18, 2024

hiteshk25 commented Jan 22, 2024

noblepaul commented Jan 22, 2024

hiteshk25 commented Jan 22, 2024

noblepaul commented Nov 20, 2023 •

edited by chatman

Loading

hiteshk25 commented Dec 8, 2023 •

edited

Loading

patsonluk Jan 18, 2024 •

edited

Loading

noblepaul Jan 22, 2024 •

edited

Loading

patsonluk Jan 18, 2024 •

edited

Loading